Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Device support for the Xiaomi Mi Smart Fan #52

Merged
merged 3 commits into from
Sep 5, 2017

Conversation

syssi
Copy link
Collaborator

@syssi syssi commented Sep 3, 2017

Retry.

@coveralls
Copy link

coveralls commented Sep 3, 2017

Coverage Status

Coverage increased (+1.8%) to 32.771% when pulling 07b1c88 on syssi:feature/fan-pr into 89f4a6c on rytilahti:master.

self.natural_level, self.speed_level, self.oscillate,
self.battery, self.ac_power, self.poweroff_time,
self.speed_level, self.angle)
return s
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure if it makes sense to expose everything in __str__ for all devices, but that can be cleaned up for all supported devices at some point if needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright.

mirobo/fan.py Outdated
@property
def temperature(self) -> float:
if self.data["temp_dec"] is not None:
return self.data["temp_dec"] / 10.0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires return None in the end to avoid missing "return" (typing, flake8 etc. can be run by simply calling tox in the source dir btw).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My IDE is unhappy now because None isn't "float". ;-)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it should be defined as Optional[float] then :-)

mirobo/fan.py Outdated
@property
def led_brightness(self) -> LedBrightness:
if self.data["led_b"] is not None:
return LedBrightness(self.data["led_b"])
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here, just add return None to the end.

@rytilahti
Copy link
Owner

rytilahti commented Sep 3, 2017

Looks much better now, thanks! Just a couple of nits to be done before merging. I'll probably have to look into how the output of mypy/flake8 can be integrated here directly to checks.

@syssi
Copy link
Collaborator Author

syssi commented Sep 4, 2017

Could you just tell me the commands I can execute locally to check?

@coveralls
Copy link

coveralls commented Sep 4, 2017

Coverage Status

Coverage increased (+1.8%) to 32.732% when pulling ca10a8b on syssi:feature/fan-pr into 89f4a6c on rytilahti:master.

@rytilahti
Copy link
Owner

Could you just tell me the commands I can execute locally to check?

Executing tox on the source directory will run py.test, flake8 and mypy accordingly. Here's how to run those manually (outside of tox's env):
mypy: mypy --ignore-missing-imports mirobo
flake8: flake8 mirobo
py.test: pytest mirobo

mirobo/fan.py Outdated
if self.data["temp_dec"] is not None:
return self.data["temp_dec"] / 10.0
else:
return None
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to skip the else block completely and just return False if the if doesn't get hit.

@coveralls
Copy link

coveralls commented Sep 5, 2017

Coverage Status

Coverage increased (+1.8%) to 32.732% when pulling e99be3b on syssi:feature/fan-pr into 89f4a6c on rytilahti:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+1.8%) to 32.732% when pulling e99be3b on syssi:feature/fan-pr into 89f4a6c on rytilahti:master.

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@rytilahti rytilahti merged commit 3bde912 into rytilahti:master Sep 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants